Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Complete rebranding of bibtexkey as citationkey #6875

Merged
merged 2 commits into from
Sep 8, 2020
Merged

Complete rebranding of bibtexkey as citationkey #6875

merged 2 commits into from
Sep 8, 2020

Conversation

tobiasdiez
Copy link
Member

Follow-up from #6545.

  • Change in CHANGELOG.md described (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Sep 4, 2020
@koppor
Copy link
Member

koppor commented Sep 4, 2020

The JabRef maintainers will add the following name to the AUTHORS file. In case you want to use a different one, please comment here and adjust your name in your git configuration for future commits.

MLEP

tobiasdiez
tobiasdiez commented Sep 4, 2020
View reviewed changes
src/main/java/org/jabref/model/entry/field/InternalField.java
@@ -37,6 +37,11 @@
}

public static Optional<InternalField> fromName(String name) {
if (name.equalsIgnoreCase("bibtexkey")) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migration of preferences etc is not necessary because we have such a beautiful architecture that every field name is passed trough here. 😆 😄

@tobiasdiez tobiasdiez mentioned this pull request Sep 4, 2020
Merged
@Siedlerchr
Copy link
Member

Siedlerchr commented Sep 5, 2020 •
edited
Loading

In general looks good, but I would prefer a preferences migration for the preview and the default patterns etc to be consistent. Otherwise it looks confusing when we always refer to citation key but some pieces still use bibtexkey
Did you test if custom layout files still work with \bibtexkey ?

Siedlerchr
Siedlerchr requested changes Sep 5, 2020
View reviewed changes
Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please test if the layout files still work and follow the checklist.

@tobiasdiez
Copy link
Member Author

I left the bibtexkey in the old version of the migration tests since this is the form they are encountered usually for a user, so the tests are really covering the use case.

The custom layout files I didn't check, but with custom preview it did worked.

@calixtus
Copy link
Member

calixtus commented Sep 8, 2020

We should merge this soon, so conflicts with new PRs can be avoided. I scrolled through this one, but did not see anything.

@tobiasdiez
Copy link
Member Author

Agreed. Let's merge it now, and hopefully everything keeps working. If not, feel free to assign the new issues on my name ;-)

@tobiasdiez tobiasdiez merged commit 6dec174 into master Sep 8, 2020
@tobiasdiez tobiasdiez deleted the citekey branch September 8, 2020 13:34
@Siedlerchr
Copy link
Member

There are failing tests related to the Preferences MIgration:
https://github.com/JabRef/jabref/runs/1086155398


jabRefPreferences.setPreviewStyle(
    "<font face="sans-serif"><b><i>\bibtextype</i><a name="\citationkey">\begin{citationkey} (\citationkey)</a>\end{citationkey}</b><br>__NEWLINE__\begin{author} \format[Authors(LastFirst,Initials,Semicolon,Amp),HTMLChars]{\author}<BR>\end{author}__NEWLINE__\begin{editor} \format[Authors(LastFirst,Initials,Semicolon,Amp),HTMLChars]{\editor} <i>(\format[IfPlural(Eds.,Ed.)]{\editor})</i><BR>\end{editor}__NEWLINE__\begin{title} \format[HTMLChars]{\title} \end{title}<BR>__NEWLINE__\begin{chapter} \format[HTMLChars]{\chapter}<BR>\end{chapter}__NEWLINE__\begin{journal} <em>\format[HTMLChars]{\journal}, </em>\end{journal}__NEWLINE__\begin{booktitle} <em>\format[HTMLChars]{\booktitle}, </em>\end{booktitle}__NEWLINE__\begin{school} <em>\format[HTMLChars]{\school}, </em>\end{school}__NEWLINE__\begin{institution} <em>\format[HTMLChars]{\institution}, </em>\end{institution}__NEWLINE__\begin{publisher} <em>\format[HTMLChars]{\publisher}, </em>\end{publisher}__NEWLINE__\begin{year}<b>\year</b>\end{year}\begin{volume}<i>, \volume</i>\end{volume}\begin{pages}, \format[FormatPagesForHTML]{\pages} \end{pages}__NEWLINE__\begin{abstract}<BR><BR><b>Abstract: </b> \format[HTMLChars]{\abstract} \end{abstract}__NEWLINE__\begin{comment}<BR><BR><b>Comment: </b> \format[Markdown,HTMLChars]{\comment} \end{comment}</dd>__NEWLINE__<p></p></font>"
);
-> at org.jabref.migrations.PreferencesMigrationsTest.upgradePreviewStyleAllowMarkupDefault(PreferencesMigrationsTest.java:169)
Actual invocations have different arguments:
jabRefPreferences.setPreviewStyle(
    "<font face="sans-serif"><b><i>\bibtextype</i><a name="\bibtexkey">\begin{bibtexkey} (\bibtexkey)</a>\end{bibtexkey}</b><br>__NEWLINE__\begin{author} \format[Authors(LastFirst,Initials,Semicolon,Amp),HTMLChars]{\author}<BR>\end{author}__NEWLINE__\begin{editor} \format[Authors(LastFirst,Initials,Semicolon,Amp),HTMLChars]{\editor} <i>(\format[IfPlural(Eds.,Ed.)]{\editor})</i><BR>\end{editor}__NEWLINE__\begin{title} \format[HTMLChars]{\title} \end{title}<BR>__NEWLINE__\begin{chapter} \format[HTMLChars]{\chapter}<BR>\end{chapter}__NEWLINE__\begin{journal} <em>\format[HTMLChars]{\journal}, </em>\end{journal}__NEWLINE__\begin{booktitle} <em>\format[HTMLChars]{\booktitle}, </em>\end{booktitle}__NEWLINE__\begin{school} <em>\format[HTMLChars]{\school}, </em>\end{school}__NEWLINE__\begin{institution} <em>\format[HTMLChars]{\institution}, </em>\end{institution}__NEWLINE__\begin{publisher} <em>\format[HTMLChars]{\publisher}, </em>\end{publisher}__NEWLINE__\begin{year}<b>\year</b>\end{year}\begin{volume}<i>, \volume</i>\end{volume}\begin{pages}, \format[FormatPagesForHTML]{\pages} \end{pages}__NEWLINE__\begin{abstract}<BR><BR><b>Abstract: </b> \format[HTMLChars]{\abstract} \end{abstract}__NEWLINE__\begin{comment}<BR><BR><b>Comment: </b> \format[HTMLChars]{\comment} \end{comment}</dd>__NEWLINE__<p></p></font>"
);
-> at org.jabref.migrations.PreferencesMigrationsTest.upgradePreviewStyleAllowMarkupDefault(PreferencesMigrationsTest.java:164)
jabRefPreferences.getPreviewStyle(

Siedlerchr added a commit that referenced this pull request Sep 12, 2020
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into fixCopyPasteCodeArea
897f5e0 
* upstream/master:
  Fix sort order in main table not stored (#6898)
  Window "Manage field names & content": make title consistent with menu (+ localization) (#6895)
  Complete rebranding of `bibtexkey` as `citationkey` (#6875)
  Dev-doc: ADR list (#6887)
  Bump lucene-queryparser from 8.6.1 to 8.6.2 (#6881)
  Bump fontbox from 2.0.20 to 2.0.21 (#6883)
  Bump org.beryx.jlink from 2.21.3 to 2.21.4 (#6882)
  Bump com.github.ben-manes.versions from 0.29.0 to 0.30.0 (#6884)
  Bump pdfbox from 2.0.20 to 2.0.21 (#6885)
  Bump mockito-core from 3.5.7 to 3.5.10 (#6886)
  lint changelog
  Dev Doc: Moving around ADR (end?) (#6873)
  Add MD031 to lint ignore list
  Add mlep's other email adress
  Fetch by ID: SAO/NASA Astrophysics Data System -> SAO/NASA ADS (#6876)
Siedlerchr added a commit that referenced this pull request Sep 12, 2020
@Siedlerchr
Merge remote-tracking branch 'upstream/master' into testEA15Adopt
784c025 
* upstream/master: (55 commits)
  Fix sort order in main table not stored (#6898)
  Window "Manage field names & content": make title consistent with menu (+ localization) (#6895)
  Complete rebranding of `bibtexkey` as `citationkey` (#6875)
  Dev-doc: ADR list (#6887)
  Bump lucene-queryparser from 8.6.1 to 8.6.2 (#6881)
  Bump fontbox from 2.0.20 to 2.0.21 (#6883)
  Bump org.beryx.jlink from 2.21.3 to 2.21.4 (#6882)
  Bump com.github.ben-manes.versions from 0.29.0 to 0.30.0 (#6884)
  Bump pdfbox from 2.0.20 to 2.0.21 (#6885)
  Bump mockito-core from 3.5.7 to 3.5.10 (#6886)
  lint changelog
  Dev Doc: Moving around ADR (end?) (#6873)
  Add MD031 to lint ignore list
  Add mlep's other email adress
  Fetch by ID: SAO/NASA Astrophysics Data System -> SAO/NASA ADS (#6876)
  Reimport -> Reload (#6874)
  GitBook: [master] 19 pages and 4 assets modified
  Add new author
  Delete .Rhistory
  Fix icon issue (#6858)
  ...
@calixtus calixtus mentioned this pull request Sep 15, 2020
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Siedlerchr Siedlerchr Siedlerchr requested changes

Assignees
No one assigned
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tobiasdiez @koppor @Siedlerchr @calixtus